fix URLSession crashing on HTTP/0.9 simple-responses #1097

weissi · 2017-07-06T10:35:45Z

also add tests for misbehaving HTTP servers.

we discovered that if the server is just returning random bytes, the whole app would crash with fatalError("Received body data, but the header is not complete, yet.") . The reason we can get into that state is that CURL support HTTP/0.9 simple-responses which are just the body bytes...

The issue is also discussed on CURL's github.

To make sure we don't regress and to document the current behaviour, I also wrote tests for a few other cases when the HTTP server is misbehaving or behaving oddly.

weissi · 2017-07-06T10:47:50Z

@swift-ci please test

alblue · 2017-07-06T12:08:04Z

TestFoundation/TestNSURLSession.swift

@@ -41,6 +41,10 @@ class TestURLSession : XCTestCase {
 	    ("test_customProtocolResponseWithDelegate", test_customProtocolResponseWithDelegate),
            ("test_httpRedirection", test_httpRedirection),
            ("test_httpRedirectionTimeout", test_httpRedirectionTimeout),
+            ("test_illegalHTTPServerResponses", test_illegalHTTPServerResponses),


If we're keeping the order of the tests the same as in the test list, this line should be moved to the end.

thanks @alblue , addressed

weissi · 2017-07-06T12:38:18Z

@swift-ci please test

weissi · 2017-07-06T14:54:46Z

@swift-ci please test

pushkarnk · 2017-07-07T07:16:58Z

This is interesting. The change and the tests look good to me.

Does Foundation on Darwin also gracefully handle such responses?

weissi · 2017-07-07T07:40:56Z

@pushkarnk two intentions in this pull request

most importantly don't just crash the program on unexpected server response as that's terrible for the user
document the current behaviour in unit tests

I haven't tried how regular Foundation behaves, but it certainly won't just crash ;). Do we want to match regular Foundation exactly or what is the intention here? With my patch we handle these not so common server responses just the way CURL handles them which is what we seem to do usually in Swift Foundation's URLSession.

weissi · 2017-07-07T07:45:54Z

@pushkarnk @parkera @phausler My suggestion would be to get this patch in as soon as possible to be sure it'll land in Swift 4 as it's just terrible if the program crashes if the server misbehaves. Worst is that if you connect to HTTP2-only servers you might hit that behaviour (as a HTTP2 only response looks like a HTTP/0.9 simple-response to CURL versions without HTTP2 support).

After we addressed the crasher issues we should talk about what we want to achieve here and how we want to handle all the special cases. See here for the list of odd server behaviours I could quickly come up with. We should then go through that list one by one and decide what the appropriate behaviour should be.

But IMHO more importantly we should get something in to fix the crashes. We can then always decide later that we want to throw away HTTP/0.9 simple responses even though CURL handles them.

pushkarnk · 2017-07-07T09:06:49Z

Specifically, if the server is using HTTP/1.1 but the header is malformed (as you've demonstrated in the tests) does Darwin also return an HTTPURLResponse with httpVersion set to 0.9?

pushkarnk · 2017-07-07T09:08:18Z

Do we want to match regular Foundation exactly or what is the intention here?

Yes, we'd ideally want to. Unless we are sure that the Darwin Foundation isn't doing the right thing

pushkarnk · 2017-07-07T09:21:51Z

Foundation/NSURLSession/http/HTTPURLProtocol.swift

-        guard ts.isHeaderComplete else { fatalError("Received body data, but the header is not complete, yet.") }
+        guard case .transferInProgress(var ts) = internalState else { fatalError("Received body data, but no transfer in progress.") }
+        if !ts.isHeaderComplete {
+            ts.response = HTTPURLResponse(url: ts.url, statusCode: 200, httpVersion: "0.9", headerFields: [:])


Is this valid in the three different scenarios that you have pointed out?

When the server sends a HTTP/0.9 response aka "only the body"

When the server is running HTTP/1.1 but sends a malformed response

When the server running HTTP/2 sends a response to client running HTTP/1.1
... I'm specifically concerned about the httpVersion: "0.9"

pushkarnk · 2017-07-07T09:30:16Z

In short, I seem to have only two questions:

Is the behaviour consistent with Darwin? (Do we desire that? I'd say yes!)
Is the HTTPURLResponse being returned valid in all the failure scenarios pointed out?

Having said that, I agree that we need to fix these crashes first.
@parkera @phausler What would you suggest?

weissi · 2017-07-07T09:37:13Z

@pushkarnk so in Darwin Foundation, it's really a NSURLHTTPResponse that's created using a CFURLHTTPResponse that's created with a CFHTTPMessageCreateResponse and that is initialised with the following parameters:

* thread #2, queue = 'com.apple.CFNetwork.LoaderQ', stop reason = breakpoint 1.5
    frame #0: 0x00007fff2d1be705 CFNetwork`CFHTTPMessageCreateResponse
CFNetwork`CFHTTPMessageCreateResponse:
->  0x7fff2d1be705 <+0>: pushq  %rbp
    0x7fff2d1be706 <+1>: movq   %rsp, %rbp
    0x7fff2d1be709 <+4>: pushq  %r15
    0x7fff2d1be70b <+6>: pushq  %r14
Target 0: (URLSessionRepro) stopped.
(lldb) po $rdi
\<CFAllocator 0x7fff579b1f10 [0x7fff579b1f10]>{info = 0x7fff5def2000}

(lldb) po $rsi
200

(lldb) po $rdx
<nil>

(lldb) po $rcx
HTTP/0.9

(lldb)

so it does indeed treat it as HTTP/0.9 too but the string seems to be HTTP/0.9 not just 0.9. I will change my patch accordingly.

also add tests for misbehaving HTTP servers.

weissi · 2017-07-07T09:40:28Z

@pushkarnk also the NSURLHTTPResponse doesn't actually seem to have a property so it seems like you won't be able to query the HTTP version anyway but I guess we want it compatible.

weissi · 2017-07-07T09:40:35Z

@swift-ci please test

pushkarnk · 2017-07-07T09:44:54Z

@swift-ci please test

weissi · 2017-07-08T16:34:55Z

@pushkarnk tests seemed to have passed. Any further comments/questions? @phausler happy with this?

phausler · 2017-07-09T16:09:45Z

The change looks sensible to me

also add tests for misbehaving HTTP servers.

weissi requested review from phausler and pushkarnk July 6, 2017 10:47

alblue reviewed Jul 6, 2017

View reviewed changes

pushkarnk reviewed Jul 7, 2017

View reviewed changes

fix URLSession crashing on HTTP/0.9 simple-responses

5d206fb

also add tests for misbehaving HTTP servers.

pushkarnk requested review from pushkarnk and removed request for pushkarnk July 7, 2017 09:52

phausler merged commit 79b511a into swiftlang:master Jul 9, 2017

kevints pushed a commit to kevints/swift-corelibs-foundation that referenced this pull request Aug 1, 2017

fix URLSession crashing on HTTP/0.9 simple-responses (swiftlang#1097)

c3f0735

also add tests for misbehaving HTTP servers.

fix URLSession crashing on HTTP/0.9 simple-responses #1097

fix URLSession crashing on HTTP/0.9 simple-responses #1097

Uh oh!

Conversation

weissi commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weissi commented Jul 6, 2017

Uh oh!

alblue Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

weissi Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

weissi commented Jul 6, 2017

Uh oh!

weissi commented Jul 6, 2017

Uh oh!

pushkarnk commented Jul 7, 2017

Uh oh!

weissi commented Jul 7, 2017

Uh oh!

weissi commented Jul 7, 2017

Uh oh!

pushkarnk commented Jul 7, 2017

Uh oh!

pushkarnk commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pushkarnk Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weissi commented Jul 7, 2017

Uh oh!

weissi commented Jul 7, 2017

Uh oh!

weissi commented Jul 7, 2017

Uh oh!

pushkarnk commented Jul 7, 2017

Uh oh!

weissi commented Jul 8, 2017

Uh oh!

phausler commented Jul 9, 2017

Uh oh!

Uh oh!

weissi commented Jul 6, 2017 •

edited

Loading

pushkarnk commented Jul 7, 2017 •

edited

Loading

pushkarnk Jul 7, 2017 •

edited

Loading

pushkarnk commented Jul 7, 2017 •

edited

Loading